💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 453e043945

Review: Add fingerprint-based module caching (#3576)

Overall this is a well-designed feature. It reuses the existing IModuleResultRepository extension point cleanly rather than inventing a parallel mechanism, the fingerprint composition (module identity + inputs + key parts + env vars + dependency results) is thorough, the filesystem/S3/Redis stores all use atomic write patterns (temp file + rename, or write-chunks-then-publish-metadata), and the test suite covers the tricky cases well (fingerprint captured pre-execution even when the module mutates its own input, dependency value changes invalidating dependents, glob file-count limits). Docs are clear and the "correctness rules" section sets the right expectations for users.

A few things worth addressing before merge:

1. _fingerprints dictionary leaks entries whenever a cached module doesn't finish Successful (src/ModularPipelines/Caching/ModuleCacheResultRepository.cs:61, 109, 127, 169)

GetResultAsync stores the computed fingerprint in _fingerprints[module] on a cache miss (line 127) so SaveResultAsync can reuse it later instead of re-hashing inputs. But SaveResultAsync only removes that entry inside the try/finally at line 109 — and it early-returns at line 61 before reaching that finally whenever moduleResult.ModuleStatus != Status.Successful. Looking at the callers in ModuleExecutionPipeline: SaveToHistory is invoked for Successful and IgnoredFailure results, but for a plain Failed result it's never called at all. So for any cache-enabled module that fails (with or without an ignore-failure condition), the dictionary keeps a strong reference to that IModule for the remainder of the repository's lifetime.

Why this matters: it's not just a style nit — it's the guard clause silently defeating the cleanup the finally block was written to guarantee. In a typical one-shot CLI pipeline run the blast radius is small (process exits anyway), but it will bite in any host that reuses the same DI container/repository across multiple pipeline executions (e.g. a persistent build server), and it's the kind of latent bug that's easy to reintroduce again if someone refactors this method without noticing the ordering dependency.

Suggestion: wrap the whole method body in try/finally (or use _fingerprints.TryRemove unconditionally in a finally at the top of the method) so cleanup doesn't depend on which branch returns. That also removes the implicit coupling between "when does this get called" in ModuleExecutionPipeline and "when does this get cleaned up" in the repository.

2. Fingerprint validation is copy-pasted across all three store implementations

FileSystemModuleCache.GetEntryPath (src/ModularPipelines/Caching/FileSystemModuleCache.cs:66), S3ModuleCache.ValidateFingerprint (src/ModularPipelines.Distributed.Artifacts.S3/Caching/S3ModuleCache.cs:109-114), and RedisModuleCache.ValidateFingerprint (src/ModularPipelines.Distributed.Redis/Caching/RedisModuleCache.cs:191-196) all contain the identical fingerprint.Length != 64 || fingerprint.Any(c => !Uri.IsHexDigit(c)) check and the same error message.

Why this matters: these three packages already share IModuleCacheStore from ModularPipelines.Caching. Since the S3 and Redis projects already reference that assembly, a single internal static class ModuleCacheFingerprint { public static void Validate(string fingerprint) } (or make it public since it's a useful guard for third-party IModuleCacheStore implementations too) in the core package would let all three call the same method. Right now, fixing or extending the validation rule (e.g. supporting a future non-SHA-256 hash, or improving the exception message) requires three synchronized edits, and it's easy for one to drift.

3. Undocumented trust in mtime+size for skipping re-hashes (src/ModularPipelines/Caching/ModuleCacheFileHasher.cs:42-43)

HashAsync skips re-hashing a file when its Length and LastWriteTimeUtc.Ticks match the persisted index — a reasonable, common optimization (make/ccache/incremental-build tools all do this), but it means the fingerprint doesn't strictly reflect current file content as the docs (docs/docs/how-to/module-caching.md:44-49) state — it reflects content-as-of-last-observed-mtime-change. A tool that rewrites a file in place while preserving both size and timestamp (some editors, some VCS checkout operations, or filesystems with coarse timestamp resolution) would produce a silent stale cache hit rather than a cache miss, which is the worse failure mode for a caching feature explicitly built around a "Correctness rules" section.

Suggestion: not necessarily a blocker, but the "Correctness rules" doc section should call this trade-off out explicitly (the same way build tools that make this trade-off document it), so users know why touching a file without changing bytes is safe but some external tools that preserve stat metadata across writes are not.

4. Duplicate/garbled XML doc summaries (multiple files)

Several new constructors have both the British and American spelling of the summary line, e.g.:

• src/ModularPipelines/Caching/FileSystemModuleCache.cs:13-14
• src/ModularPipelines.Distributed.Artifacts.S3/Caching/S3ModuleCache.cs:20-21
• src/ModularPipelines.Distributed.Redis/Caching/RedisModuleCache.cs:20-21
• src/ModularPipelines/Attributes/CacheInputsAttribute.cs:19-20

/// Initialises a new instance of the <see cref="FileSystemModuleCache"/> class.
/// Initializes a new instance of the <see cref="FileSystemModuleCache"/> class.

Looks like leftover from an automated spelling pass that appended rather than replaced. Worth a quick cleanup pass before merge since these are public API docs.

Nice addition overall — the store abstraction is clean and the S3/Redis backends correctly avoid coupling cache keys to distributed run identifiers so they're safely shareable across concurrent runs. Items 1 and 2 are the ones I'd actually want fixed before merge; 3 and 4 are lower priority polish.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9250bfa1da

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed after the fix(caching): harden cache correctness (b772837) and Merge remote-tracking branch 'origin/main' (8a857f6) commits, checking against the four items raised in my prior review on commit 453e043945.

Previously flagged items — status

1. _fingerprints dictionary leak on failed modules — ✅ Fixed. ModuleCacheResultRepository.SaveResultAsync now wraps its whole body in try/finally with DiscardFingerprint(module) in the finally (src/ModularPipelines/Caching/ModuleCacheResultRepository.cs:63-120), and ModuleExecutionPipeline also unconditionally calls _cacheResultRepository?.DiscardFingerprint(module) in its own outer finally (src/ModularPipelines/Engine/ModuleExecutionPipeline.cs:202) regardless of module outcome. Cleanup no longer depends on which branch returns.

2. Duplicated fingerprint validation across stores — ✅ Fixed. All three stores (FileSystemModuleCache, S3ModuleCache, RedisModuleCache) now call the shared ModuleCacheFingerprint.Validate(...) (src/ModularPipelines/Caching/ModuleCacheFingerprint.cs).

3. Undocumented mtime+size trust for skipping re-hashes — ✅ Resolved, and better than just documenting it: the hash-index shortcut was removed entirely. ModuleCacheFileHasher.HashAsync now always hashes actual file content and re-hashes if size/mtime changed during hashing, to detect concurrent modification (src/ModularPipelines/Caching/ModuleCacheFileHasher.cs:29-42). The new "Correctness rules" section states this explicitly ("Input files are content-hashed on every fingerprint calculation. File size and timestamps are not treated as proof that content is unchanged.").

4. Duplicate British/American XML doc summary lines — Partially cleaned up (S3ModuleCache/RedisModuleCache constructors now have a single summary line), but FileSystemModuleCache.cs:13-14 and CacheInputsAttribute.cs:19-20 still have both lines. Not a blocker — the same double-line pattern already exists throughout the pre-existing codebase (CommandBuilderBase.cs, DelegateRequirement.cs, DownloadFileOptions.cs, etc.), so this reads as an established repo convention rather than a defect introduced by this PR.

New finding

Stale doc paragraph contradicts the new "Correctness rules" section it sits above (docs/docs/how-to/module-caching.md:80):

"The file limit prevents unexpectedly broad globs. File hashes use a persistent modification-time and size index; changed files are hashed concurrently."

This describes the old mtime/size hash-index design that was removed in the "harden cache correctness" commit (item 3 above) — there's no HashIndexPath/persisted index anywhere in ModuleCacheOptions or ModuleCacheFileHasher anymore. It directly contradicts the "Correctness rules" bullet 23 lines below it, which correctly states files are always content-hashed. This is exactly the kind of doc/code drift the "Correctness rules" section is meant to prevent trust in — worth a one-line fix (e.g. "The file limit prevents unexpectedly broad globs. Changed files are hashed concurrently.") before merge so readers don't walk away with the wrong mental model of cache correctness.

Other observations (non-blocking)

• AddModuleCache<TStore> composing via Replace for IModuleCacheStore + TryAddSingleton for everything else (src/ModularPipelines/Extensions/PipelineBuilderExtensions.cs:262-278) is a clean way to make registration order-independent and to let a user-supplied IModuleCacheResultRepository win over the default — nice touch, matches the PR description's "regardless of registration order" claim.
• RedisModuleCache chunked writes under a fresh generation GUID per write, with the metadata key published last, so concurrent readers never observe a half-written entry; old-generation chunks simply expire via TTL rather than being explicitly deleted — a reasonable simplicity/storage tradeoff for a cache.
• ModuleCacheFileResolver's glob-to-regex conversion compiles with a 1-second matchTimeout, which is a good defensive touch against pathological user-supplied patterns.

Overall this is in good shape — the substantive correctness issues from the last pass are resolved. I'd only ask for the one doc line above to be fixed before merge.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a857f618c

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 73f478a4d7

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed after fix(caching): preserve artifact semantics (73f478a), checking the current HEAD against every item raised across my two prior review rounds and Codex's eight inline findings.

Status of all previously flagged items

From my review on 453e043945 and follow-up on 8a857f618c:

1. _fingerprints dictionary leak on failed modules — ✅ still fixed (SaveResultAsync's try/finally + ModuleExecutionPipeline's outer finally at ModuleExecutionPipeline.cs:205).
2. Duplicated fingerprint validation across stores — ✅ still fixed (shared ModuleCacheFingerprint.Validate).
3. Undocumented mtime+size trust for skipping re-hashes — ✅ still fixed (removed entirely; files are always content-hashed).
4. Stale doc paragraph describing the old hash-index — resolved in the doc pass.

From Codex's eight P1/P2 findings on 8a857f618c, the three called out explicitly in this PR's description are now correctly implemented and covered by new tests in ModuleCacheTests.cs:

• Cache artifacts after the after-execute hook: ModuleExecutionPipeline.cs:148-162 now runs InvokeAfterExecuteAsync before SaveResults, and only sets CompletionSource after both complete (ModuleExecutionPipeline.cs:164-165). CacheCapturesArtifactsAfterAfterExecuteHook verifies the archived artifact reflects the post-hook file content.
• Remove stale outputs before restoring cached artifacts: RestoreArtifactsAsync now calls ClearArtifacts (deleting everything matching the module's ProducesArtifact patterns) before extracting the selected entry (ModuleCacheResultRepository.cs:342). CacheRestoreRemovesArtifactsAbsentFromSelectedEntry confirms b.txt from a stale run is gone after restoring the a entry.
• Preserve executable modes when archiving artifacts: Unix mode is now captured into ZipArchiveEntry.ExternalAttributes on write and reapplied via RestoreUnixFileMode on restore (ModuleCacheResultRepository.cs:313-317, 401-419), verified by CacheRestorePreservesUnixExecutableMode.

The remaining five Codex findings, which weren't mentioned in the PR description, also check out against the current code:

• Exclude cache directory from input expansion: ComputeFingerprintAsync and AddArtifactsAsync/ClearArtifacts all pass _options.CacheDirectory as excludedDirectory into ModuleCacheFileResolver.ResolveFiles, which skips it (ModuleCacheFileResolver.cs:110-120).
• Preserve existing result repositories when enabling caching: AddModuleCache never touches IModuleResultRepository; it registers a separate IModuleCacheResultRepository, and ModuleExecutionPipeline consumes both independently (ModuleExecutionPipeline.cs:34-48). Registration order between AddResultsRepository and AddModuleCache no longer matters because they're disjoint DI service types.
• Pass pipeline cancellation to cache operations: the token now threads end-to-end — TryGetCachedResult/SaveResults pass executionContext.ModuleCancellationTokenSource.Token through to IModuleCacheResultRepository, and FileSystemModuleCache/S3ModuleCache/RedisModuleCache all forward it into the underlying I/O calls (GetObjectAsync, PutObjectAsync, StringGetAsync/StringSetAsync via WaitAsync(cancellationToken)).
• Isolate Redis cache options from distributed services: AddRedisModuleCache uses a private keyed IConnectionMultiplexer (ModuleCacheConnectionKey) and builds RedisModuleCache from locally-scoped option instances rather than registering RedisDistributedOptions/ArtifactOptions as unkeyed DI singletons, so it no longer collides with AddRedisDistributedCoordinator/AddRedisDistributedArtifactStore.
• Isolate S3 cache configuration from artifact storage: AddS3ModuleCache likewise captures S3ArtifactOptions in a closure for S3ModuleCache instead of registering it in DI, so it can't collide with AddS3DistributedArtifactStore's separate registration.

I also did a clean local build of the touched files: core ModularPipelines.sln -c Release builds with 0 warnings / 0 errors on this branch.

One thing worth a sanity check, not a blocker

Moving module.CompletionSource.TrySetResult(...) to after SaveResults (ModuleExecutionPipeline.cs:157-165) is the right call for cache correctness — dependents must never observe a module as "done" before its cache/history write captures the post-hook artifact state. But SaveResults also drives the plain IModuleResultRepository history path, so this now inserts a synchronization point between completion and dependent start for every module using history persistence, not just cache-enabled ones. That's probably fine (SaveResultAsync failures are caught/logged rather than propagated, so it won't hang), but it's a behavior change with a blast radius slightly wider than the caching feature itself — worth a one-line callout in the PR description or changelog so it doesn't read as a silent latency change to existing history-repository users.

Overall

This is a clean, well-tested piece of work. The store abstraction (IModuleCacheStore) stays uniform across filesystem/S3/Redis, the fingerprint composition is thorough (module identity + inputs + key parts + env vars + dependency results/exceptions/skip-reasons), and every substantive correctness issue raised across three rounds of review has been addressed with a corresponding regression test rather than just a fix. Nothing outstanding blocks merge from my side.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1843f68ce7

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed the final commit, dbca45a081 ("fix(cache): honor skips and empty directories") plus the trailing merge (1843f68ce7, HEAD), which weren't covered by my prior two review rounds on 453e043945 / 8a857f618c / 73f478a4d7. This commit addresses the two remaining open Codex findings from that round.

Verified: the two remaining Codex findings are correctly fixed

1. "Evaluate skip conditions before accepting cache hits" — Fixed by simple reordering in ModuleExecutionPipeline.ExecuteAsync (src/ModularPipelines/Engine/ModuleExecutionPipeline.cs:75-102): the fluent/dependency skip-decision block now runs before the config.CacheEnabled cache-lookup block (previously it ran after). A WithSkipWhen gate now always wins over a cache hit, matching normal (non-cached) module semantics. Confirmed by the new FluentSkipConditionTakesPrecedenceOverCacheHit test, which asserts the second run reports Status.Skipped with reason "gate closed" rather than reusing the cached artifact, and that the module only executed once.

2. "Preserve empty directories in artifact snapshots" — Fixed via a new ModuleCacheFileResolver.ResolveDirectories (mirrors ResolveFiles) that walks declared ProducesArtifact patterns for directories, not just files. AddArtifactsAsync now writes explicit trailing-/ ZIP directory entries for each resolved directory; RestoreArtifactsAsync recreates those directories before extracting file entries; and ClearArtifacts now also deletes directories that are empty after stale files are removed (ordered deepest-first via OrderByDescending(path => path.Length), which is a valid proxy for depth since a child path is always longer than its parent). Confirmed by the new CacheRestorePreservesEmptyArtifactDirectories test — deletes the artifact tree, re-runs, and asserts the nested empty directory reappears via UsedHistory restoration without re-executing the module.

Both fixes are minimal, targeted, and each ships with a regression test that would fail without the fix. I also did a clean local build and test run to double-check:

• ModularPipelines.sln -c Release: 0 warnings, 0 errors
• ModuleCacheTests: 15/15 passed
• ModuleExecutionPipelineTests: 3/3 passed

One non-blocking observation (new, not raised before)

ModuleCacheFileResolver.ResolveDirectories duplicates almost all of ResolveFiles (src/ModularPipelines/Caching/ModuleCacheFileResolver.cs:8-116) — pattern normalization, glob-regex construction, root-containment checks, and the limit-enforcing Add* helper are all copy-pasted with only the filesystem predicate (File.Exists/Directory.Exists) and enumeration call (EnumerateFiles/EnumerateDirectories) differing. This is the same shape of duplication flagged (and fixed, via ModuleCacheFingerprint.Validate) earlier in this PR for the three cache-store implementations. A shared private helper parameterized on Func<string,bool> exists + Func<string,IEnumerable<string>> enumerate would collapse both methods to a few lines each and remove the risk of the two traversal implementations drifting apart (e.g. if the containment-escape check ever needs a fix, someone has to remember there are two copies). Not a blocker — the two copies are currently in sync — but worth cleaning up given the precedent already set in this same PR.

Overall

This closes out every substantive issue raised across three review rounds (mine and Codex's), each with a corresponding test. Nothing outstanding blocks merge from my side; the duplication note above is a nice-to-have for a follow-up, not this PR.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e31f4fdbc8

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed after the latest commit, e31f4fdb ("fix(cache): preserve hook and directory modes"), which addresses my prior P1 finding about await this deadlocking in a self-awaiting OnAfterExecuteAsync hook, plus the Unix directory-mode preservation follow-up. The directory-mode fix looks solid: modes are now applied to directories deepest-first (OrderByDescending(artifact => artifact.Destination.Length)) so a restrictive parent mode can never block traversal into a child before its mode is restored.

However, the deadlock fix itself introduces a new correctness regression:

OnAfterExecuteAsync result transformations are no longer visible to dependent modules

In ModuleExecutionPipeline.cs (around line 147-166), the fix now does:

moduleResult = ModuleResult<T>.CreateSuccess(result, executionContext);
executionContext.SetTypedResult(moduleResult);
module.CompletionSource.TrySetResult(moduleResult);   // <-- published with PRE-hook result

afterHookInvoked = true;
moduleResult = await InvokeAfterExecuteAsync(...)      // <-- hook may return a transformed result
    .ConfigureAwait(false);

await SaveResults(...).ConfigureAwait(false);

executionContext.SetTypedResult(moduleResult);          // <-- executionContext gets POST-hook result
// module.CompletionSource is NOT updated again
return moduleResult;

Module<T>.GetAwaiter() resolves via CompletionSource.Task (src/ModularPipelines/Modules/Module.cs:271), and this is exactly what dependent modules consume — e.g. await context.GetModule<PackProjectsModule>() in PackagePathsParserModule.cs. Since TrySetResult only succeeds once, CompletionSource is now permanently pinned to the pre-hook result, while executionContext's typed result (used for history/cache saving and the pipeline summary) reflects the post-hook result. OnAfterExecuteAsync is explicitly documented as being able to "add cleanup or result transformation" and return a modified result — but any such transformation will now silently never reach a dependent module that awaits the module directly, even though the cached/saved/reported result is the transformed one. This is a real behavioral divergence between what gets persisted/reported and what dependents observe, not just an edge case.

The existing test added for this fix (OnAfterExecuteAsync_Can_Await_Its_Own_Module) doesn't catch this because the hook just does return await this; (returning the same value already published), and the older ResultModifyingModule test explicitly notes "We can't easily modify the internal result here, so we just verify the hook was called" — so there's no coverage asserting a genuinely transformed result propagates to a dependent module.

Suggested fix: keep publishing the pre-hook result early to unblock self-awaiting hooks (that part is correct), but if the hook actually returns a different result, republish it — e.g. reset/replace CompletionSource's value isn't possible on TaskCompletionSource once set, so this likely needs a different mechanism: either (a) only pre-publish when the hook is detected as self-awaiting (hard to detect generically), or (b) have dependents read the result through executionContext's typed-result task instead of module.CompletionSource so there's a single source of truth that's updated once, after the hook completes, with CompletionSource reserved only for unblocking a self-await. Option (b) is preferable because it keeps a single authoritative result path (cache/history/summary/dependents all read the same post-hook value) rather than two diverging result channels.

Everything else from earlier rounds (fingerprint cleanup ordering, skip-before-cache-lookup, empty-directory artifact handling, executable bit preservation) still looks correctly addressed at this commit.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e52a73f19

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3c76d3037

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed at HEAD (cc7d20af), which lands on top of the last claude[bot] review round at b3c76d3037 (2026-08-02T10:38). That round flagged two things: (1) unbounded S3 object buffering as a P1, and (2) a non-blocking architectural note about ModuleCacheResultRepository size. Checked both directly against current source.

1. S3 unbounded-buffering finding — fixed correctly

fix(cache): bound S3 cache downloads (cc7d20af) closes this properly, with defense in depth rather than a single check:

• OpenReadAsync now rejects up front when response.ContentLength > _maximumCacheEntryBytes (S3ModuleCache.cs:70-73).
• The actual copy no longer trusts that server-reported length — CopyResponseToAsync enforces the same limit while streaming, checking totalBytes > _maximumCacheEntryBytes - bytesRead before each write (correctly avoids the overflow a naive totalBytes + bytesRead > max could hit, and fails closed the moment the running total would exceed the cap rather than after the fact).
• MaximumCacheEntryBytes is now threaded from ModuleCacheOptions through DI (AddS3ModuleCache pulls IOptions<ModuleCacheOptions>) into both public and internal S3ModuleCache constructors, with a ValidateCacheOptions guard rejecting a non-positive limit.
• Covered by a new regression test, OpenReadRejectsContentAboveConfiguredLimit, which sets the limit to 2 bytes and asserts the specific exception message.

This is a real fix at the point of the network read, exactly matching what the prior review asked for.

2. The Redis analog flagged in the same review round is still open

The same 10:20:54Z review explicitly called this out as worth checking "while you're in this area": RedisModuleCache.OpenReadAsync (src/ModularPipelines.Distributed.Redis/Caching/RedisModuleCache.cs:59-105) parses chunkCount/expectedLength from Redis-stored metadata and then loops _database.StringGetAsync for every chunk, writing each straight to a local temp file — with no size or count check anywhere in the loop. expectedLength is only compared against the actual bytes written after the entire entry has already been downloaded to disk (stream.Length != expectedLength, line 98). RedisModuleCache doesn't even accept a ModuleCacheOptions/limit parameter — unlike the constructor S3ModuleCache just gained in this same commit, RedisModuleCache's constructor and its AddRedisModuleCache DI wiring (RedisDistributedExtensions.cs:42-47) never reference MaximumCacheEntryBytes at all.

Since this PR's own docs describe the Redis store as shareable across concurrent/multi-tenant pipeline runs — the same threat model that motivated the S3 fix — a corrupted or oversized shared entry can still exhaust local disk on the Redis path before any framework guard fires, via the same "trust the store's stated size, download first, validate after" shape the S3 fix just eliminated. Given the fix pattern already exists one file over in the same PR (limit threaded through DI, checked both up front via reported length and while streaming), it would be a small, consistent addition to apply the same _maximumCacheEntryBytes guard here — e.g. reject if chunkCount * _chunkSize (or expectedLength) exceeds the limit before starting the download loop, and/or track running bytes written per chunk the same way CopyResponseToAsync does.

3. Leftover minor nit from the same round, still present

RedisModuleCache.cs:20-22 still has the duplicate XML doc summary flagged in the 10:00:49Z review (Initialises... / Initializes... back to back on the constructor). Harmless, but since it's a one-line fix that was already called out, worth cleaning up in the same pass as the item above rather than leaving it to drift further.

Not re-litigated

The architectural note from the last round (ModuleCacheResultRepository doing fingerprinting + serialization + ZIP/archive + rollback in one ~1,300-line class) was explicitly flagged as a non-blocking maintainability suggestion, not a correctness issue — no change expected here, and none was made. Everything else verified in the last several rounds (ZIP entry-count bound via ZipCentralDirectory, matching save/restore MaximumArtifactEntries, Redis provisional chunk TTLs, the $valueType deserialization restriction) still holds at this HEAD.

Summary

Good, targeted fix for the one blocking item from the last round, with a test that pins the exact failure mode. Before merge I'd still want the Redis side of the same class of issue closed — right now the S3 and Redis stores enforce genuinely different trust boundaries for what should be the same MaximumCacheEntryBytes contract.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc7d20af5a

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b0b45dc00

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d2b614373

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7536977d26

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed at HEAD (7536977), which adds one commit (fix(cache): harden artifact restoration) on top of the last review round at 3d2b6143.

New commit — reviewed, no bugs found

• ModuleCacheFileResolver.IsWithinDeclaredArtifactScope: wildcard patterns (*, **/*) no longer match the working-directory root itself (relativePath == "."); only an exact . declaration now scopes the root as an artifact. Matches the commit message ("require exact root declarations") and is covered by the new ArtifactScopeRequiresExactDeclarationForWorkingRoot test, which asserts */**/* return false for the root while . returns true.
• ModuleCacheResultRepository.SaveResultAsync: the result is now serialized to a plain temp file first, its length is checked against MaximumResultBytes, and the cache write is skipped entirely (with a debug log) if it's too large — the zip is only built afterward via the new WriteCacheEntryAsync. Previously only artifacts had a size ceiling; a module returning a huge in-memory result had no bound on the cache entry it produced. Verified the two temp files (serializedResult, temporary) are each cleaned up in their own finally, so there's no leak on the early-return path. Covered by CacheSaveSkipsResultAboveConfiguredLimit.
• Symbolic-link target confinement: artifact-restore now reads each symlink's target once up front, validates it via the new ValidateSymbolicLinkTarget (resolves the target, requires it stay under root via IsWithin, then walks every path component between root and the target rejecting any that's already a reparse point on disk), and caches the target in a dictionary for the later restore pass instead of re-reading the zip entry. This closes a real gap — a target that's textually inside root but reachable only by first passing through an existing on-disk symlink could previously escape. Covered by two new tests: CacheRestoreRejectsSymbolicLinkTargetsOutsideWorkingDirectory and CacheRestoreRejectsTargetsThroughExistingSymbolicLinks, both asserting the pipeline falls back to re-execution rather than failing outright or leaking outside the working tree.

Walked through the boundary cases by hand (link target equal to root, relative .. targets, targets behind a pre-existing symlink) and the logic holds. ModularPipelines.sln (core) builds clean at this HEAD with 0 warnings/errors.

Simplification opportunity (new in this commit)

ValidateSymbolicLinkTarget's component-walk loop (ModuleCacheResultRepository.cs, checking each path segment between root and the resolved target for TryGetReparsePointAttributes) duplicates the same walk already implemented in RemoveLinkedDestinationComponents a few dozen lines above it — same split-and-Path.Combine traversal, same TryGetReparsePointAttributes check per component, differing only in what happens when a reparse point is found (throw vs. delete). This PR already consolidated an equivalent duplication once before (refactor(cache): share artifact path traversal, and the prior review round called out IsWithin consolidation positively) — worth extracting a shared EnumeratePathComponents(root, relativePath) (or similar) helper that both callers use, so the "walk from root to target, stop at the first reparse point" logic exists in exactly one place. This isn't a correctness bug, just a maintainability point: two independent copies of a security-sensitive traversal are two places to keep in sync if either needs a fix later (e.g., the Windows AltDirectorySeparatorChar handling, or the "."-component skip that only one of the two copies has today).

Still open from prior review rounds

Reflection-based value deserialization in ModuleResult.cs (DeserializeSuccessValue) — unchanged by this commit. It still resolves the cache/wire-supplied $valueType via Type.GetType(valueTypeName, throwOnError: false) and only checks DeclaredValueType.IsAssignableFrom(valueType), whereas ExceptionJsonConverter in the same file uses the stricter Namespace.StartsWith("System") allow-list pattern for the equivalent problem. If DeclaredValueType is broad (an interface or object), a poisoned cache entry (shared Redis/S3 store, or a tampered local file) could still select any assignable type from loaded assemblies to deserialize into. This was flagged in the previous review round as needing an explicit decision before merge; it's still outstanding.

Summary

This commit is a solid, well-tested, narrowly-scoped hardening pass — no new issues. The one item worth resolving (or explicitly accepting as out of scope) before merge is the Type.GetType gap in ModuleResult.cs, carried over from the last review round.

Follow-up review: fingerprint-based module caching (#3576)

Re-reviewed at HEAD (ed4d99d13a), which adds one commit (fix(cache): bound published cache entries) on top of the last review round at 7536977.

New commit — reviewed, no bugs found

• ModuleCacheResultRepository.WriteCacheEntryAsync now checks the built archive's length against the new MaximumCacheEntryBytes (default 10 GiB) and returns false to skip the cache write (with a debug log) instead of publishing an unbounded entry. This is a sensible aggregate ceiling on top of the existing streaming ArtifactByteBudget/MaximumArtifactBytes enforcement during archive construction — the two aren't redundant, since compression means the on-disk archive size isn't a simple function of the pre-compression artifact byte budget. The skip path was verified to still run DiscardFingerprint via the outer finally in SaveResultAsync, and both temp files (serializedResult, temporary) are cleaned up on the early-return path, so there's no leak or stale-fingerprint risk.
• Path comparer consolidation: the private, hardcoded PathComparer in ModuleCacheResultRepository (OperatingSystem.IsWindows() ? OrdinalIgnoreCase : Ordinal) is replaced with the shared ModuleCacheFileResolver.GetPathComparer(root), which probes actual on-disk case sensitivity per working-directory volume rather than assuming it from the OS. This is a good DRY fix — it removes a second, less-accurate implementation of the same concept and matches the consolidation pattern this PR has favored in earlier rounds (e.g. the shared ResolvePaths/IsWithin helpers).
• Tests were refactored to share a single AssertCacheSaveSkippedAsync helper parameterized over maximumCacheEntryBytes/maximumResultBytes, covering both the pre-existing result-size limit and the new archive-size limit (CacheSaveSkipsResultAboveConfiguredLimit, CacheSaveSkipsEntryAboveConfiguredLimit). Read through both — they correctly assert no cache entry is written and that a second run re-executes the module (status Successful both times, ExecutionCount == 2).

Walked the full diff (7536977..ed4d99d13a) — only ModuleCacheResultRepository.cs and ModuleCacheTests.cs changed, both narrowly scoped to this fix. ModularPipelines.sln core builds clean at this HEAD.

Still open from the last review round

Reflection-based value deserialization in ModuleResult.cs (DeserializeSuccessValue) is unchanged by this commit. It still resolves the cache/wire-supplied $valueType via Type.GetType(valueTypeName, throwOnError: false) and only checks DeclaredValueType.IsAssignableFrom(valueType), while ExceptionJsonConverter in the same file uses a stricter allow-list pattern (Namespace.StartsWith("System")) for the equivalent problem. Given this PR ships shared, cross-run cache backends (S3, Redis) as a first-class scenario, a poisoned cache entry could still influence which type gets instantiated when DeclaredValueType is broad (an interface or object). This was flagged as needing an explicit decision before merge in the previous round — worth resolving (tighten the allow-list to match ExceptionJsonConverter, or explicitly document why it's out of scope) before merge, since it's the one remaining gap in an otherwise very thorough path/type-safety pass across this feature.

Summary

No new issues in this commit — it's a clean, well-tested hardening/refactor pass consistent with the rest of the PR. The one carried-over item worth closing out before merge is the Type.GetType allow-list gap in ModuleResult.cs.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ed4d99d13a